Performance: collapse 19 builtin transforms into a single Call dispatcher - #3167
Performance: collapse 19 builtin transforms into a single Call dispatcher#3167Pierre-Sassoulas wants to merge 2 commits into
Conversation
``register_builtin_transform`` registered 19 separate transforms on ``nodes.Call``. The transform visitor ran all 19 predicates per Call node, each repeating the same ``isinstance``/name checks before all but one bailed out. Route by name through a dict instead, so a Call node pays for the checks once. The registration API is unchanged: ``register_builtin_transform`` now fills the dispatch table, so adding a builtin still takes one call and new entries enroll automatically. Refs #1115
Merging this PR will improve performance by 5.64%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | test_bench_endtoend_parse_flask |
5.3 s | 4.9 s | +6.43% |
| ⚡ | Simulation | test_bench_endtoend_walk_infer_flask |
23.1 s | 21.9 s | +5.57% |
| ⚡ | Simulation | test_bench_endtoend_walk_infer_black |
35.7 s | 34 s | +4.95% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf/call-dispatcher (e4bb930) with main (da4a8cf)
Footnotes
-
1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3167 +/- ##
==========================================
- Coverage 93.67% 93.66% -0.01%
==========================================
Files 93 93
Lines 11645 11657 +12
==========================================
+ Hits 10908 10919 +11
- Misses 737 738 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
DanielNoord
left a comment
DanielNoord
left a comment
There was a problem hiding this comment.
Pretty hard to wrap you head around this haha
Tried my best, haven't looked at the tests yet
| # Builtins inference — registered through a single dispatcher to avoid | ||
| # running 19 separate predicates on every Call node walked by the | ||
| # transform visitor (#1115). | ||
| manager.register_transform( | ||
| nodes.Call, | ||
| inference_tip(_builtin_dispatch_transform), | ||
| _builtin_dispatch_predicate, | ||
| ) | ||
| register_builtin_transform(manager, infer_bool, "bool") | ||
| register_builtin_transform(manager, infer_super, "super") | ||
| register_builtin_transform(manager, infer_callable, "callable") |
There was a problem hiding this comment.
Why are we still registering the transforms here?
There was a problem hiding this comment.
Didn't want to modify the API for register_builtin_transform (no leading underscore). If we remove this constraint, there's a lot we can do in a simpler way.
There was a problem hiding this comment.
I don't expect non core astroid to ever need to call register_builtin_transform, so I'd be fine with changing the API :)
| inference_tip(_transform_wrapper), | ||
| partial(_builtin_filter_predicate, builtin_name=builtin_name), | ||
| ) | ||
| del manager # No longer needed; dispatcher is registered once globally. |
There was a problem hiding this comment.
This feels dangerous. Is the del really necessary?
There was a problem hiding this comment.
No, we could use a noqa instead. But the other discussion might make this one obsolete.
2 participants
Type of Changes
Description
Split out of #3048, as requested — one optimization, one commit, tests included.
register_builtin_transformregistered 19 separate transforms onnodes.Call. The transform visitor ran all 19 predicates perCallnode, each repeating the sameisinstance/name checks before all but one bailed out. This routes by name through a dict instead, so aCallnode pays for those checks once.The registration API is unchanged:
register_builtin_transformnow fills the dispatch table, so adding a builtin still takes one call and new entries enroll automatically.tests/test_transforms.pygains coverage for the dispatcher: routing by name, unknown names falling through untouched, and the inference tips still firing for each registered builtin.Refs #1115